frontend/app/tab/workspaceswitcher.scss (1)

15-20: Consider removing !important if possible.

The !important declarations work but can make future style overrides difficult. If these are needed to override styles from a parent component or library, consider increasing selector specificity instead (e.g., .tab-bar-wrapper .workspace-switcher-button). Otherwise, the transparent background with subtle hover effect nicely matches the VS Code aesthetic.

frontend/app/tab/tab-menu.ts (1)

129-136: onApply callback executes even if UpdateObjectMeta fails.

The onApply callback runs after UpdateObjectMeta regardless of whether the update succeeded. If the update throws, fireAndForget swallows the error, but onApply won't execute since the exception aborts the async function. However, if you want explicit error handling or user feedback on failure, consider wrapping with try/catch.

💡 Optional: Add error handling for better UX

             click: () =>
                 fireAndForget(async () => {
-                    // Sanitize preset to ensure only allowed keys are sent
-                    const sanitizedPreset = sanitizePreset(presetName, preset);
-                    await ObjectService.UpdateObjectMeta(oref, sanitizedPreset);
-                    onApply?.(presetName);
+                    try {
+                        const sanitizedPreset = sanitizePreset(presetName, preset);
+                        await ObjectService.UpdateObjectMeta(oref, sanitizedPreset);
+                        onApply?.(presetName);
+                    } catch (e) {
+                        console.error(`[Preset] Failed to apply "${presetName}":`, e);
+                    }
                 }),

pkg/waveobj/validators.go (3)

145-162: Path existence check has TOCTOU implications.

The os.Stat call on lines 146-162 creates a time-of-check-time-of-use window. Between validation and actual use of the path, the file system state could change. Since this is soft validation (warn but allow), the impact is limited, but be aware that:

1. Non-existent paths at validation time may exist later (acceptable per comment)
2. Existing paths at validation time may not exist when used

This appears intentional given the logging approach, but consider documenting this behavior in the function's comment.

---

296-322: Integer validation silently truncates fractional values.

ValidateInt casts float64 to int at line 312 without verifying the value is a whole number. A value like 5.9 passes validation as 5, which may be unexpected.

♻️ Suggested improvement

 func ValidateInt(key string, value interface{}, minVal, maxVal int) error {
 	if value == nil {
 		return nil
 	}

 	// JSON numbers come as float64
 	f, ok := value.(float64)
 	if !ok {
 		return &ValidationError{
 			Key:     key,
 			Value:   value,
 			Message: fmt.Sprintf("must be a number, got %T", value),
 		}
 	}

 	i := int(f)
+	if float64(i) != f {
+		return &ValidationError{
+			Key:     key,
+			Value:   f,
+			Message: "must be an integer, got floating point value",
+		}
+	}
 	if i < minVal || i > maxVal {

---

853-872: Consider adding scope validation for AI presets.

PresetKeyScope defines allowed keys for tabvar@ and bg@ prefixes, but ai@ presets are actively used (referenced in CountCustomAIPresets in settingsconfig.go) and have well-defined keys (MetaKey_AiBaseURL, MetaKey_AiName, MetaKey_AiModel, etc.). Currently, ai@ presets bypass validation with only a warning. Adding them to PresetKeyScope would enforce consistency with other preset types.

pkg/wconfig/defaultconfig/presets/tabvars.json (1)

1-10: Consider clarifying these are example presets.

These presets reference specific paths (~/Code/waveterm, ~/Projects/myapp) that likely won't exist for most users. Since the path validation allows non-existent paths with warnings, this won't cause errors, but users may be confused by seeing these defaults.

Consider either:

1. Removing these defaults so users start with an empty preset file
2. Adding a comment (if JSON comments are supported) or documentation explaining these are examples
3. Renaming to clearly indicate they're examples (e.g., tabvar@example-waveterm-dev)

frontend/app/store/keymodel.ts (1)

335-362: Consider validating tab basedir only when you actually need it.
Right now validation runs before checking the focused block’s cmd:cwd, which can delay shortcuts unnecessarily (especially for network paths). You could first attempt the focused block cwd, and only validate tab basedir if it’s still needed.

Also applies to: 379-382

pkg/wstore/wstore.go (2)

62-65: Silently ignored error from DBGetORef.

The error returned by DBGetORef is discarded. While this follows the existing pattern in UpdateObjectMeta (line 39), database errors could be silently masked. Consider logging or returning the error if non-nil.

♻️ Suggested improvement

-		obj, _ := DBGetORef(tx.Context(), oref)
+		obj, err := DBGetORef(tx.Context(), oref)
+		if err != nil {
+			return fmt.Errorf("error fetching object: %w", err)
+		}
 		if obj == nil {
 			return ErrNotFound
 		}

---

104-108: Consider clearer error wrapping order.

The error wrapping fmt.Errorf("%w: %w", ErrVersionMismatch, ErrObjectLocked) puts ErrVersionMismatch as the primary error. Since the actual cause is a lock conflict, callers using errors.Is(err, ErrObjectLocked) will still work, but semantically ErrObjectLocked is the specific cause. Consider swapping the order or using a single sentinel error.

♻️ Alternative wrapping

-			return fmt.Errorf("%w: %w", ErrVersionMismatch, ErrObjectLocked)
+			return fmt.Errorf("%w: %w", ErrObjectLocked, ErrVersionMismatch)

frontend/util/pathutil.ts (3)

152-155: Control character regex is intentional - acknowledge static analysis.

The regex /[\x00-\x1F]/ intentionally detects control characters (null byte through unit separator) which is correct for security validation. The Biome linter warning is a false positive in this context.

Consider adding a comment to document this intentional usage:

📝 Document intentional usage

         // Control characters (0x00-0x1F)
+        // Note: Intentionally checking for control characters - linter warning is expected
         if (/[\x00-\x1F]/.test(path)) {
             return true;
         }

---

125-129: Potential false positive for // paths on Unix.

The pattern /^\/\/[^\/]/ flags paths starting with // followed by a non-slash, which could match legitimate Unix paths. While //foo is technically valid on some Unix systems (and often normalized to /foo), this is a reasonable security tradeoff for blocking URL-style UNC paths.

Consider limiting this check to Windows only if false positives become an issue:

     // URL-style UNC that might slip through: //server/share
     // (if it starts with // followed by non-slash)
-    if (/^\/\/[^\/]/.test(path)) {
+    if (PLATFORM === PlatformWindows && /^\/\/[^\/]/.test(path)) {
         return true;
     }

---

48-72: Consider adding higher-numbered COM/LPT ports.

Windows supports COM10-COM256 and LPT10-LPT256 as device names in some contexts. The current list covers the most common cases, but a more robust approach could use a regex pattern.

♻️ Alternative using regex in isWindowsDeviceName

-const WINDOWS_DEVICE_NAMES = [
-    "CON",
-    "PRN",
-    "AUX",
-    "NUL",
-    "COM1",
-    // ... etc
-];
+const WINDOWS_DEVICE_NAME_PATTERN = /^(CON|PRN|AUX|NUL|COM\d+|LPT\d+)$/i;

 function isWindowsDeviceName(path: string): boolean {
     const parts = path.split(/[\/\\]/);
     const filename = parts[parts.length - 1] || path;
     const nameWithoutExt = filename.split(".")[0].toUpperCase();
-    return WINDOWS_DEVICE_NAMES.includes(nameWithoutExt);
+    return WINDOWS_DEVICE_NAME_PATTERN.test(nameWithoutExt);
 }

frontend/app/tab/tab.tsx (1)

81-89: Consider adding error handling for the auto-clear update.

The ObjectService.UpdateObjectMeta call inside the timeout lacks error handling. While unlikely to fail, unhandled promise rejections could be silently swallowed.

♻️ Add error handling

                     const timer = setTimeout(() => {
-                        ObjectService.UpdateObjectMeta(makeORef("tab", id), {
-                            "tab:termstatus": null,
-                        });
+                        ObjectService.UpdateObjectMeta(makeORef("tab", id), {
+                            "tab:termstatus": null,
+                        }).catch((err) => console.warn("Failed to clear tab status:", err));
                     }, delay);

frontend/app/view/term/termwrap.ts (2)

316-319: Dynamic import inside hot path may cause timing inconsistencies.

The dynamic import("@/store/tab-model") inside the debounced callback executes on every successful OSC 7 update. While this works, it adds latency and the import might resolve at different times.

Consider importing getTabModelByTabId statically at the top of the file since tab-model is likely already bundled with the frontend.

Suggested change

Add at top of file:

import { getTabModelByTabId } from "@/store/tab-model";

Then replace lines 316-319:

-                            const { getTabModelByTabId } = await import("@/store/tab-model");
-                            const tabModel = getTabModelByTabId(tabId);
+                            const tabModel = getTabModelByTabId(tabId);

---

188-191: Consider removing or reducing verbose console.log in production.

Lines 188 and 190 log every OSC 7 event. While useful for debugging, this may generate excessive console noise in production.

Suggested change

-    console.log("OSC 7 received:", { data, blockId, loaded });
+    dlog("OSC 7 received:", { data, blockId, loaded });
     if (!loaded) {
-        console.log("OSC 7 ignored - terminal not loaded");
+        dlog("OSC 7 ignored - terminal not loaded");
         return true;
     }

frontend/app/store/tab-basedir-validator.ts (2)

171-189: Unused tabId parameter in validateTabBasedir.

The tabId parameter is declared but never used in the function body. It appears to be vestigial from an earlier design.

Suggested fix

 export async function validateTabBasedir(
-    tabId: string,
     basedir: string
 ): Promise<PathValidationResult> {

Note: This will require updating all call sites. Alternatively, prefix with underscore to indicate intentionally unused:

 export async function validateTabBasedir(
-    tabId: string,
+    _tabId: string,
     basedir: string
 ): Promise<PathValidationResult> {

---

289-295: Module-level mutable state may cause issues with concurrent validation flows.

The batchingState singleton is shared across all validation calls. If the module is imported multiple times (e.g., in tests or hot-reload scenarios), each import shares the same state, which could lead to unexpected behavior.

For production use this is likely fine, but consider documenting this singleton behavior.

frontend/app/store/tab-model.ts (1)

39-44: Add lifecycle cleanup to prevent TabModel cache accumulation in long sessions.

The tabModelCache global Map accumulates TabModel instances without any removal mechanism. When tabs are closed via removeTabView(), the TabModel and its terminalStatusMap remain cached indefinitely. While individual blocks are cleaned up via removeBlockTerminalStatus(), the entire TabModel instance is never disposed. In long sessions with many tab open/close cycles, this leads to unbounded memory growth.

Consider adding a cache eviction mechanism or explicit cleanup (e.g., tabModelCache.delete(tabId)) triggered when tabs are removed.